Skip to content

[SYCL] Add XFAIL trackers for several E2E tests marked with XFAIL #16287

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
aelovikov-intel merged 11 commits into from
Dec 6, 2024
Merged

[SYCL] Add XFAIL trackers for several E2E tests marked with XFAIL #16287

aelovikov-intel merged 11 commits into from
Dec 6, 2024

Conversation

@lbushi25
Copy link
Contributor

@lbushi25 lbushi25 commented Dec 5, 2024 •
edited
Loading

This PR is the first in a series of PR's to add XFAIL tracker issues for several tests marked as XFAIL on certain platforms.
For XFAIL's on a hip_nvidia platform, we simply remove the XFAIL entirely as we do not test the configuration anymore.
Also, this PR deleted 6 tests failing everywhere because of an issue that will not be fixed.

@lbushi25 lbushi25 marked this pull request as ready for review December 5, 2024 15:27
@lbushi25 lbushi25 requested review from a team as code owners December 5, 2024 15:27
aelovikov-intel
aelovikov-intel reviewed Dec 5, 2024
View reviewed changes
sycl/test-e2e/Basic/span.cpp Outdated
//
// Fails to release USM pointer on HIP for NVIDIA
// XFAIL: hip_nvidia
// XFAIL-TRACKER: https://github.com/intel/llvm/issues/14404
Copy link
Contributor

@aelovikov-intel aelovikov-intel Dec 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this correct? The issue is about AMD HIP, not NVidia.

Copy link
Contributor Author

@lbushi25 lbushi25 Dec 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the catch, fixed it!

Copy link
Contributor

@aelovikov-intel aelovikov-intel Dec 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see how this issue is useful though. If the HW is really unsupported, it should be a REQUIRES: !arch-<something> or something like that.

Or, going deeper, I don't see a value in just adding a bunch of formal links if they don't become actionable.

+ @AlexeySachkov

Copy link
Contributor

@AlexeySachkov AlexeySachkov Dec 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tend to agree with Andrei here. If we don't support this configuration (and AFAIK, we don't), then there is no reason for us to maintain those XFAILs, we can just drop them.

sarnex
sarnex reviewed Dec 5, 2024
View reviewed changes
aelovikov-intel
aelovikov-intel reviewed Dec 5, 2024
View reviewed changes
Copy link
Contributor

@aelovikov-intel aelovikov-intel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If nobody is working on hip_nvidia and that isn't tested anywhere, maybe we should remove XFAILs instead?

sarnex
sarnex approved these changes Dec 5, 2024
View reviewed changes
Copy link
Contributor

@sarnex sarnex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

esimd lgtm

@sarnex sarnex requested a review from a team December 5, 2024 16:32
aelovikov-intel
aelovikov-intel reviewed Dec 6, 2024
View reviewed changes
sycl/test-e2e/Basic/queue/release.cpp Outdated
// RUN: env SYCL_UR_TRACE=2 %{run} %t.out | FileCheck %s %if !windows %{--check-prefixes=CHECK-RELEASE%}
//
// XFAIL: hip_nvidia
// XFAIL-TRACKER: https://github.com/intel/llvm/issues/16197
Copy link
Contributor

@aelovikov-intel aelovikov-intel Dec 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we remove XFAIL instead?

Copy link
Contributor Author

@lbushi25 lbushi25 Dec 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we remove XFAIL instead?

Yes, this one slipped through the cracks. Fixed it now!

@lbushi25 lbushi25 requested a review from bader as a code owner December 6, 2024 16:19
@bader bader removed their request for review December 6, 2024 17:11
@aelovikov-intel aelovikov-intel merged commit b0000e0 into intel:sycl Dec 6, 2024
14 checks passed
@lbushi25 lbushi25 mentioned this pull request Dec 9, 2024
Merged
aelovikov-intel pushed a commit that referenced this pull request Dec 10, 2024
@lbushi25
[SYCL] Add XFAIL trackers for E2E tests and remove hip_nvidia XFAILs …
6a9de65 
…entirely (#16307)

This is a cleanup of several tests marked as XFAIL on certain platforms.
It is a follow up of #16287 and it
follows the same principles in handling of XFAILs.
@lbushi25 lbushi25 mentioned this pull request Dec 12, 2024
Merged
sommerlukas pushed a commit that referenced this pull request Dec 16, 2024
@lbushi25
[SYCL] Add XFAIL trackers to joint matrix tests (#16345)
345b054 
Follow up of #16287 to reduce the
number of improperly XFAIL-ed tests in our repo.
Thanks to @dkhaldi for the mapping between tests and trackers.
aelovikov-intel added a commit to aelovikov-intel/llvm that referenced this pull request Jan 13, 2025
@aelovikov-intel
[SYCL] Remove HIP Nvidia infrastructure support
6922ad0 
As noted in intel#14432 the configuration
isn't supported/tested anymore and there were some PRs removing related
`XFAIL`s (intel#16287,
intel#16307, and possibly others).

This PR goes a step further removes all the support for that
configuration.
@aelovikov-intel aelovikov-intel mentioned this pull request Jan 13, 2025
Merged
aelovikov-intel added a commit that referenced this pull request Jan 14, 2025
@aelovikov-intel
[SYCL] Remove HIP Nvidia tests/CI support (#16611)
96ea8dc 
As noted in #14432 the configuration
isn't supported/tested anymore and there were some PRs removing related
`XFAIL`s (#16287,
#16307, and possibly others).

This PR goes a step further and removes all the support for that
configuration in our LIT tests.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sarnex sarnex sarnex approved these changes

@aelovikov-intel aelovikov-intel aelovikov-intel approved these changes

@AlexeySachkov AlexeySachkov Awaiting requested review from AlexeySachkov

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@lbushi25 @AlexeySachkov @sarnex @aelovikov-intel